-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: plugin config param tooltips #30
feat: plugin config param tooltips #30
Conversation
@Keyrxng, this task has been idle for a while. Please provide an update. |
@Keyrxng, this task has been idle for a while. Please provide an update. |
2 similar comments
@Keyrxng, this task has been idle for a while. Please provide an update. |
@Keyrxng, this task has been idle for a while. Please provide an update. |
@henalolp, this task has been idle for a while. Please provide an update. |
|
Beyond the tooltips I have implemented:
I did not open a new task as I felt submitting this PR for review without the proper handling/displaying of descriptions etc was not meeting the spec and unions required additional handling, so I apologise for going off-spec a little. |
@gentlementlegen @0x4007 @rndquu - still unable to request a review so I'm having to tag, ty. |
@Keyrxng It works better after removing the configuration, although it takes a very long time to display the first screen because somehow it cannot fetch ![]() Why are some field names having dots and other having spaces? Also it seems that these kind of selectors don't work: Screen.Recording.2025-02-11.at.10.55.07.mov |
This is a little hard for me to debug. Perhaps it's a permission/visibility issue with that org?
Params which are objects use
My mistake forgot to remove the
Added a selected state
Reworded the commit to QA: |
Truly don't understand about that organization because it has very similar settings to the other ones, if I find something I'll let you know. Cool let me test again. |
Hmmm that's strange so for you, you don't see the toast notification saying that fetching is happening? Would a centered spinner or loader or something be prefered until all fetching has completed? See I only have one org to fetch from these days so it's hard for me to experience having to fetch from 3 or 4 orgs which sounds like the case for you maybe. QA:
If I understand, you are suggesting that for these textareas which accompany the Would we keep the buttons and collapse the rendered props for that union option if they disable it? And for those which cannot be disabled they'd stay rendered? If you can create a clear spec for what exactly you'd prefer over this impl that would be great dude. I done it this way for a couple of reasons but I agree it's not optimal. |
Yes I can write a spec. My idea would be that all the fields should be rendered recursively within a tree, so if you have an object within an objects within an object they all render. I think this is doable since you have the full object shape within the generated manifest. But again outside of the scope of this pull-request. also yes I have to render 5 organizations which is why it is slow. Ideally some spinner or anything that lets you know something is happening. |
Co-authored-by: Mentlegen <[email protected]>
One approval down, I'm unsure if we can merge this now or are we waiting for another? @gentlementlegen rfc please buddy |
Probably @0x4007 can take a peek and give some feedback |
@0x4007 seems awfully busy but I hope this can move forward asap. I'm free to work on other tasks I just need one of my previous to be merged. Cheers @gentlementlegen |
@0x4007 if you don't have the time with upcoming events as you said, please assign another reviewer to get the 2nd approval needed for merge. I'd trust @gentlementlegen's approval personally, the guy knows his shit but let's get this merged sooner rather than later, itching to move on. |
@whilefoo fancy taking a peak my dude? |
Okay gents -- can we merge this? Since the purpose of the PR was for the config param tooltips and I can spec a task to handle the off-spec bugs |
TY fellas. I'll do some bug hunting today, idk if one of you want to start the spec and I can update it with anything else I find? If that makes sense to do it that way I mean. |
Resolves #24
Requires PRs in plugin repos be merged in and I'll remove the mocks
Changes:
QA: